Introduce deprecated coroutine builder overloads accepting a Job#4435
Introduce deprecated coroutine builder overloads accepting a Job#4435dkhalanskyjb merged 6 commits intodevelopfrom
Conversation
|
Awesome stuff! |
| * throw IllegalStateException("$it is tired of all this") | ||
| * } | ||
| * } | ||
| * coroutines.joinAll() |
There was a problem hiding this comment.
Why do an explicit join here? supervisorScope should wait for its children, right?
There was a problem hiding this comment.
Yes, it will. The idea was to highlight how joinAll will not fail in this scenario, but this piece of documentation may not be a good place for that.
There was a problem hiding this comment.
This was also confusing to me. A simple comment on this line // joining failed supervised coroutines doesn't fail either would help!
| * scope.async(start = CoroutineStart.ATOMIC) { | ||
| * withContext(NonCancellable) { | ||
| * // this line will be reached even if the parent is cancelled | ||
| * } | ||
| * // note: the cancellation exception *can* be thrown here, | ||
| * // losing the computed value! | ||
| * } |
There was a problem hiding this comment.
Does that mean you can loose the value, even if there is no code after the withContext block?
scope.async(start = ATOMIC) {
withContext(NonCancellable) {
dontWantToLooseThisResource()
}
// no code here, or only code without suspension points
}The documentation for withContext says there will be no cancellation on entry/exit with NonCancellable.
There was a problem hiding this comment.
My comment is misleading, but yes, the value can be lost, just not because of withContext. The problem is that if the Deferred is cancelled before returning a value, it will no longer be able to finish with a value, even if the computation succeeds.
There was a problem hiding this comment.
Ok, that's a bit surprising to me. I guess it's because it could have children that haven't completed yet?
There was a problem hiding this comment.
Nope, even without any children: https://pl.kotl.in/-ZYf_PR9z
The reason can be seen in the diagram in https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines/-job/ : once a Job gets cancelled, there is no way back to being Active or Completing or returning a value.
There was a problem hiding this comment.
Makes sense, good to know 👍
There was a problem hiding this comment.
I think it'd make sense to make an overload to specifically accepts NonCancellable. It's doable since NonCancellable is not just a Job instance, but an object that has its own exclusive type.
That overload could then have the KDoc just for this use case, and could also have a matching ReplaceWith clause.
| * } finally { | ||
| * // if `await` fails because the current job is cancelled, | ||
| * // also cancel the now-unnecessary computations | ||
| * deferred.cancel() |
There was a problem hiding this comment.
Should it be pointed out that just calling cancel on deferred won't prevent the current coroutine from completing before the now cancelled coroutine in the other scope completes?
|
With these changes, Another construct could be thought out, or an overload taking the |
|
@LouisCAD, what is the compilation error? I've just checked, and |
|
Right, it compiles with just a warning. Came back here to comment about a specific use case I just had. In the snippet below, I want an async(SupervisorJob(coroutineContext.job)) { // this (the receiver) is a `CoroutineScope`
// Some throwing code
}With those changes, it'd trigger a warning, even though it's technically correct. Addressing #4329 would provide a good alternative, though. |
| /** | ||
| * Deprecated version of [future] that accepts a [Job]. | ||
| * | ||
| * See the documentation for the non-deprecated [future] function to learn about the functionality of this function. |
There was a problem hiding this comment.
Is there a way to fully-qualify the non-deprecated [future], so that it's clickable? Currently, it stays on the same page if I click on it in the kdoc in the IDE.
And the deprecated [async] below shows the non-deprecated async kdoc, confusingly.
I noticed that we don't really care about navigability of kdocs. If so, is it because of some technical difficulties outside of kx.coroutines control?
There was a problem hiding this comment.
There isn't any way to affect the resolution here, unfortunately. Not only do we not have control over this, but also, the resolution results may be different between the IDE and the rendered html documents.
In such cases, there's an argument to be made for not leaving a link at all. However, my preferred design for the improved KDoc navigability includes warnings for ambiguous cases. With that in mind, I'm hopeful that one day, after we upgrade to a new Kotlin version, we'll see a hundred "ambiguous reference in KDoc" warnings and will be able to easily audit and fix them.
| * | ||
| * It is incorrect to pass a [Job] as [context] to [produce], [async], or [launch], | ||
| * because this violates structured concurrency. | ||
| * The passed [Job] becomes the sole parent of the newly created coroutine, which completely severs the tie between |
There was a problem hiding this comment.
"completely severs the tie" -> "removes the parent-child relationship"
There was a problem hiding this comment.
The target audience for this is people who don't have a solid grasp on structured concurrency yet. To them, "parent-child relationship" may be a phrase devoid of special meaning. The dramatic "severs the tie" phrasing is intended to clearly describe the stakes: one piece of code is completely unaware of the other. The more experienced users can be expected to understand the specifics from the "sole parent" remark.
| * - The [CoroutineScope] can only complete when all its children complete. | ||
| * If the [CoroutineScope] is lexically scoped (for example, created by [coroutineScope], [supervisorScope], | ||
| * or [withContext]), this means that | ||
| * the lexical scope will only be exited (and the calling function will finish) once all child coroutines complete. |
There was a problem hiding this comment.
Points 1-3 are explanation to why "cancellation of the parent job cancels the children" is useful, and point 4 is an independent one. Consider moving points 1-3 into sub-points.
Then your structure will be: structured concurrency ensures
- A cancelled parents cancel children (A is important because 1, 2, 3)
- B parent waits for children
There was a problem hiding this comment.
See a note on lifecycle below, related to this section.
There was a problem hiding this comment.
These two notes are the only non-trivial comments, requesting changes in this section (and other sections referring to this one.)
This section establishes the ground rules, and the following sections should refer to arguments listed here.
The whole narrative is like this:
- Passing a Job explicitly prevents structured concurrency, don't do it, we are about to give you a collection of samples to use instead.
- Defining structured concurrency (parent-child relationship), various implications of parent-child relationship, and what scenarios they prevent or enable.
- List of samples, each sample demonstrates how it preserves structured concurrency.
Currently, the structured concurrency section defines things more loosely and doesn't precisely match with the arguments in the samples.
There was a problem hiding this comment.
The more robust separation would be 1,2,3-cancellation, 4-completion, because in 3, the cancellation also goes the other way, from the child to the parent. I don't understand the benefits of such a grouping, though.
In response to your proposal, I did change the structure a bit to flow more naturally, though I'm not sure that it aligns with what you had in mind.
| * | ||
| * Sometimes, it is undesirable for the child coroutine to react to the cancellation of the parent: for example, | ||
| * some computations have to be performed either way. | ||
| * `produce(NonCancellable)` or `produce(Job())` can be used to achieve this effect. |
There was a problem hiding this comment.
It's unclear if this is what you should do, or what you should avoid.
| * some computations have to be performed either way. | ||
| * `produce(NonCancellable)` or `produce(Job())` can be used to achieve this effect. | ||
| * | ||
| * Alternative approaches that preserve structured concurrency are this: |
There was a problem hiding this comment.
| * Alternative approaches that preserve structured concurrency are this: | |
| * Here's an alternative approach that preserves structured concurrency: |
| * // are only available through `channel` | ||
| * } | ||
| * } | ||
| * // this line will only be reached when the `produce` coroutine complete |
| * // this line will be reached when both `launch` and `produce` complete | ||
| * ``` | ||
| * | ||
| * All of these approaches preserve the ability of a parent to cancel the children and to wait for their completion. |
There was a problem hiding this comment.
I like this very succinct summary of why we need to strive to preserve structured concurrency.
| * | ||
| * ``` | ||
| * GlobalScope.produce { | ||
| * // this is explicitly a rogue computation |
There was a problem hiding this comment.
I don't like the term rogue. It's emotionally charged and doesn't hint at what it means. Possibly, an orphaned coroutine would work better? Did we leak this to outside documentation already, or is there a room for negotiation?
In this particular case no need to decide now, we could just say: this computation doesn't have a parent, and thus cannot be cancelled.
| * Alternative approaches that preserve structured concurrency are this: | ||
| * | ||
| * ``` | ||
| * // Guarantees the completion |
There was a problem hiding this comment.
A bit verbose given L550?
If you decide to keep, then also add it in launch.
| * throw IllegalStateException("$it is tired of all this") | ||
| * } | ||
| * } | ||
| * coroutines.joinAll() |
There was a problem hiding this comment.
This was also confusing to me. A simple comment on this line // joining failed supervised coroutines doesn't fail either would help!
This is a small step towards deprecating the practice of breaking structured concurrency by passing a `Job` to a coroutine builder. See #3670
Co-authored-by: Luca Kellermann <lukellmann@gmail.com>
1357b31 to
cd1e3b6
Compare
This is a small step towards deprecating the practice of breaking
structured concurrency by passing a
Jobto a coroutine builder.Thanks to @LouisCAD for the idea!
See #3670